Skip to content

refactor: 删除 max-loop 空概念并修复权限默认放行与 webfetch SSRF#338

Merged
phantom5099 merged 5 commits into
mainfrom
xgopilot/issue-334-1776410758
Apr 17, 2026
Merged

refactor: 删除 max-loop 空概念并修复权限默认放行与 webfetch SSRF#338
phantom5099 merged 5 commits into
mainfrom
xgopilot/issue-334-1776410758

Conversation

@fennoai

@fennoai fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

Requested by @Cai-Tang-www

Summary

  • 全面删除 runtime/controlplane 中与 max loop 相关的空概念残留(定义、分支、测试)。
  • tools.NewManagerengine=nil 默认放行改为 fail-closed,避免权限系统被静默绕过。
  • webfetch 增加目标主机安全校验,阻断 localhost、回环、链路本地、私网等地址访问,修复 SSRF 风险。
  • 补充并调整相关测试,保证改造后行为可验证。

Verification

  • go test ./...

Notes

  • 为保证现有工具行为测试可继续执行,新增 webfetch.WithUnsafeBypassTargetValidation(ctx),仅供受控测试场景使用。
  • 本次按要求不做旧兼容,直接切换到新实现。

Closes #334

…n/webfetch security

Generated with [codeagent](https://github.com/qbox/codeagent)

Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@fennoai fennoai Bot mentioned this pull request Apr 17, 2026
fennoai[bot]

This comment was marked as outdated.

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator

@xgopilot 修复comment提到的问题,然后解决冲突后推送

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

处理结果

已按 review comment 修复 webfetch 的 DNS rebinding/TOCTOU 风险,并推送到当前 PR 分支。

本次修改

  • internal/tools/webfetch/tool.gonewHTTPClient 中接入自定义 Transport.DialContext
  • 新增 resolveDialAddress(ctx, address):在真实拨号前校验目标并将域名解析为已校验 IP 后再拨号,避免“预检安全、连接时重解析到内网地址”的绕过窗口。
  • 保留原有请求前校验;测试绕过开关 WithUnsafeBypassTargetValidation(ctx) 在拨号路径同样生效(仅受控测试场景)。
  • 新增测试 TestFetchBlocksLoopbackAtDialinternal/tools/webfetch/tool_test.go),直接覆盖真实拨号路径的回环阻断。

冲突处理

  • 对比 origin/main 检查后,当前分支未出现本地冲突文件,无需额外冲突解决提交。

验证

  • go test ./internal/tools/webfetch ./internal/tools
  • go test ./...

推送信息

  • Commit: 33d39a7
  • Branch: xgopilot/issue-334-1776410758
  • Push: 已推送到 origin/xgopilot/issue-334-1776410758

View job run

xgopilot and others added 2 commits April 17, 2026 08:44
Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@phantom5099

Copy link
Copy Markdown
Collaborator

/code 修复编译错误

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

处理结果

已修复本 PR 分支的编译错误并推送。

变更说明

  • 修复 internal/tools/manager_test.go 中不存在的类型引用:
    • executorWithoutMicroCompactPolicy -> executorWithoutOptionalCompactFeatures
  • 修复两处测试构造 NewManager 时传入 nil 权限引擎导致与当前 fail-closed 语义不一致的问题:
    • 改为 mustAllowEngine(t)

验证

  • go test ./... 通过

推送信息

  • Commit: f6a8117
  • Branch: xgopilot/issue-334-1776410758

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
@codecov

codecov Bot commented Apr 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/tools/webfetch/tool.go 66.31% 24 Missing and 8 partials ⚠️

📢 Thoughts on this report? Let us know!

@phantom5099

Copy link
Copy Markdown
Collaborator

/review

@fennoai fennoai Bot deleted the xgopilot/issue-334-1776410758 branch April 17, 2026 08:58
@phantom5099 phantom5099 restored the xgopilot/issue-334-1776410758 branch April 17, 2026 09:02
@phantom5099 phantom5099 reopened this Apr 17, 2026

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 actionable code issue and 1 documentation follow-up.

  • Actionable: webfetch currently resolves DNS twice for hostname targets (validateFetchTarget and then resolveDialAddress), adding avoidable network latency per request.
  • Follow-up: behavior for blocking localhost/private/link-local targets appears changed; please ensure user-facing docs clearly describe this restriction and the testing-only bypass behavior.

return net.JoinHostPort(ip.String(), port), nil
}

lookupCtx, cancel := context.WithTimeout(ctx, time.Second)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Execute already calls validateFetchTarget, which resolves hostnames via LookupIPAddr, and this dial path resolves the hostname again in resolveDialAddress. This duplicates DNS work on every request and can add noticeable latency under load. Consider consolidating validation + final-address selection into one resolution step (or caching/share the first lookup result for the request).

@phantom5099

Copy link
Copy Markdown
Collaborator

/simplify

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

总体改动方向正确(max-loop 清理、权限 fail-closed、SSRF 防护增强),但还有两处值得修复的风险点:一个会弱化 SSRF 防护边界,另一个会引入解析后单 IP 拨号导致的可用性回归。

Comment thread internal/tools/webfetch/tool.go Outdated

const bypassTargetValidationKey validationContextKey = "webfetch_bypass_target_validation"

// WithUnsafeBypassTargetValidation 返回跳过目标地址安全校验的上下文,仅用于受控测试场景。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithUnsafeBypassTargetValidation 作为导出 API 存在于生产代码路径中,任何仓内调用方都可以显式关闭 SSRF 校验(包含 dial-time 校验)。这会把“仅测试用途”的假设变成可被误用的安全后门。建议将 bypass 能力收敛到 _test.go(测试辅助)或通过仅测试构建约束暴露,避免运行时代码可调用。

if err != nil {
return "", fmt.Errorf("%s: resolve host: %w", toolName, err)
}
for _, record := range records {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里在 DNS 结果中命中第一个非阻断 IP 就立即返回,等价于把原本的多地址拨号退化为“单 IP 一次尝试”。当该 IP 短暂不可达但同名其他记录可用时会直接失败,属于可用性回归。建议保留并尝试全部允许的解析结果(至少按顺序回退),而不是只选首个地址。

@phantom5099

Copy link
Copy Markdown
Collaborator

/code 修复最近两个comment提到的问题

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

处理结果

已修复“最近两个 comment”指出的问题,并推送到当前 PR 分支。

变更说明

  • 关闭生产路径的 bypass 暴露:
    • 删除 internal/tools/webfetch/tool.go 中导出的 WithUnsafeBypassTargetValidation,避免运行时代码直接关闭 SSRF 校验。
    • 测试绕过改为包内测试通过上下文键注入,不再暴露生产可调用 API。
  • 修复多 IP 可用性回退:
    • resolveDialAddress 重构为 resolveDialAddresses,返回全部允许的候选地址(过滤本地/私网等受限地址)。
    • 新增 dialFirstReachable,按解析顺序逐个拨号,前序地址失败时回退后续地址,避免退化为“单 IP 一次尝试”。
  • 测试同步更新:
    • internal/tools/webfetch/tool_test.go 新增覆盖:允许地址集合过滤、空解析/全阻断分支、多地址回退拨号行为。
    • internal/app/bootstrap_test.go 调整为直接断言 webfetch 工具注入配置(MaxResponseBytes),移除对生产 bypass API 的依赖。

验证

  • go test ./... 通过

推送信息

  • Commit: ca909c6
  • Branch: xgopilot/issue-334-1776410758
  • Push: 已推送到 origin/xgopilot/issue-334-1776410758

未执行项

  • 未处理本次触发请求之外的其他 review 建议。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
@phantom5099 phantom5099 merged commit 613182b into main Apr 17, 2026
1 of 2 checks passed
@fennoai fennoai Bot deleted the xgopilot/issue-334-1776410758 branch April 17, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

查错查架构

3 participants